Delay FC for outgoing payments #4140
Conversation
Previously `should_broadcast_holder_commitment_txn` would FC a channel if an outbound HTLC that hasn't been resolved was `LATENCY_GRACE_PERIOD_BLOCKS` past expiry. For outgoing payments that won't have an inbound HTLC, block time to allow before we FC could be higher since we are not in a race to claim an inbound HTLC. Here we use 2016 blocks which is roughly 2 weeks. For cases in which a node has been offline for a while, this could help to fail the HTLC on reconnection instead of causing a FC.
|
👋 Hi! I see this is a draft PR. |
| } | ||
|
|
||
| #[test] | ||
| fn test_no_fc_outgoing_payment() { |
There was a problem hiding this comment.
It would be nice to test the async payment version. It should be easy to copy the held_htlc_timeout test and add a disconnect + connect extra blocks until the HTLC fully times out, and ensure the HTLC is failed back on disconnect without FC.
| true | ||
| }; | ||
| if (htlc_outbound && has_incoming && htlc.0.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) || | ||
| (htlc_outbound && !has_incoming && htlc.0.cltv_expiry + 2016 <= height) || |
There was a problem hiding this comment.
I think per the issue, we shouldn't FC at all for these timed out outbound payments. I can't really see a downside to doing that since the user can FC manually or contact their counterparty out-of-band, though I could be missing something. It should be really rare/weird for the counterparty to not just fail the HTLC back on reconnect anyway.
Asked @wpaulino offline and he seemed to confirm this approach
There was a problem hiding this comment.
ok, will remove the arbitrary 2016 I added. Yeah I thought about not FC at all but wasn't sure in case something weird happens and counterparty never fails it back and could end up with the HTLC stuck there. But yeah, it can FC manually.
Resolves #4048
This would give often-offline nodes roughly 2 weeks to come back online to try salvage channel
and not FC in case they had a pending payment that ultimately failed.
I'm not entirely sure if this is the right solution. This causes some tests that expect force-closures after
LATENCY_GRACE_PERIOD_BLOCKSfor outgoing payments to fail. Opening in draft just to see if I can get review on if this is ok to delay FC for outgoing payments where we don't have an inbound HTLC to claim.